-
Notifications
You must be signed in to change notification settings - Fork 721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x64 JIT support for virtual threads (Loom) #15552
Conversation
Jenkins test sanity all jdk17,jdk19 |
Jenkins test sanity xlinux,xmac,win jdk17 |
Jenkins test sanity xlinux jdk17,jdk19 |
* thisThreadGetOwnedMonitorCountOffset() * thisThreadGetCallOutCountOffset() Signed-off-by: Daryl Maier <maier@ca.ibm.com>
* callOutCount * ownedMonitorCount Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Required for virtual thread support. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Only required for hard realtime support, which has been long deprecated. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
This label is not needed any longer and its presence is interfering with adding monitor counters for virtual threads. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Required for virtual thread support. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Jenkins test sanity win,xlinux,osx jdk17,jdk19 |
@BradleyWood @r30shah : could I ask you to review this please? Additional internal testing on JDK 17 and 19 on all platforms has succeeded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have briefly looked at the counter update code and the common API added for the Loom in JIT, looks good to me, just one minor question I have posted.
@@ -1350,6 +1350,18 @@ TR::Register *J9::X86::AMD64::JNILinkage::buildDirectJNIDispatch(TR::Node *callN | |||
|
|||
if (isGPUHelper) | |||
callNode->setSymbolReference(callSymRef); //change back to callSymRef afterwards | |||
|
|||
#if JAVA_SPEC_VERSION >= 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On monitor entry and exit code, we only update the counter if it is >= Java19 and 64-Bit platform, should we be guarding the code with 64-Bit platform check as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This file is specific to 64-bit only (AMD64) and an additional 64-bit check would be redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have look at that more carefully, thanks a lot for pointing this out.
Though, I actually want to see if @gacholio's suggestion on Z PR for Loom #15752 (comment) also applies to all other codegens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really a policy decision, not a technical one. It's quite unlikely that 32-bit will make a return, though Z is perhaps the one place where it might be requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @gacholio for the explanation. It makes sense.
@joransiu since you spent time reviewing the corresponding Z changes I'm wondering if you'd be able to merge this PR when you have a chance? Thanks! |
@joransiu : thanks for the review. Would you mind merging please? Rahil has approved it as well. |
ownedMonitorCount
J9VMThread field on monitor enter/exitcallOutCount
J9VMThread field for JNI dispatchIssue: #15175